-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify Renderer discrepancy #4102
Conversation
@WinterSilence @kenjis anything to add? |
@MGatner class ViewConfig extends BaseConfig
{
/**
* @var boolean ...
*/
public $saveData = false;
....
} and replace CodeIgniter4/system/View/View.php Line 145 in 0489678
$this->saveData = (bool) $config->saveData;
Also, need fix |
@WinterSilence Thank you, all good fixes. Please re-review. |
* | ||
* @var boolean | ||
*/ | ||
public $saveData = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's can be false
by default - RendererInterface::render(string $view, array $options = null, bool $saveData = false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true
in app/Config/View.php which extends this, so I think it makes more sense to mirror that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MGatner why not same to RendererInterface::render()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue, I didn't write any of this. Probably a mistake or miscommunication among earlier devs. At this point though we have a public interface which we can't change and an App config file which we could change but would need to note in docs - I think keeping the config files consistent even though they don't match the interface makes the most sense given the mess it already is.
Description
Fixes a discrepancy between the interface docblock and the actual method definitions. Clarifies the usage in
View
.Checklist: